-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #2016 update the Python versions used by the METplus analysis t… #2044
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bikegeek. Thank you for all of your work on this task. For some of the versions changed below, I see some discrepancies in the various requirements files. I'll put comments directly on those items below. There are some other packages that weren't changed that I have some questions about, which I'll list below in the comments here:
cartopy>=0.18.0 - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or METplotpy from the "METplus component" column. It looks like this is still used in METplus wrappers for use cases.
cmocean - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not being used, please remove this row entirely.
eofs - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or METplotpy from the "METplus component" column. It looks like this is still used in METplus wrappers for use cases.
nc-time-axis 1.4 - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not being used, please remove this row entirely.
scikit-learn 0.23.2 - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or METplotpy from the "METplus component" column. It looks like this is still used in METplus wrappers for use cases.
yaml - Is this the same as pyyaml? I see that pyyaml lists https://github.com/yaml/pyyaml as the source by yaml lists https://pypi.org/project/PyYAML/. If they are the same, please pick whichever line is most accurate and delete the other.
@@ -223,7 +223,7 @@ METplus Components Python Requirements | |||
and much more easier | |||
- | |||
* - imageio | |||
- | |||
- >=2.19.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 2.19.3 in plotpy requirements.txt, but 2.6.1 in calcpy requirements.txt. Should we update the calcpy requirements.txt file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue in METcalcpy to update the versions in the requirements.txt:
dtcenter/METcalcpy#276
@@ -349,7 +349,7 @@ METplus Components Python Requirements | |||
(metplotpy) | |||
<../generated/model_applications/s2s/TCGen_fcstGFSO_obsBDECKS_GDF_TDF.html>`_ | |||
* - metpy | |||
- | |||
- >=1.3.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 1.3.1 in calcpy and plotpy nco_requirements.txt, but as 1.1.0 in calcpy requirements.txt. Should we update the calcpy requirements.txt file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/Users_Guide/overview.rst
Outdated
@@ -371,7 +371,7 @@ METplus Components Python Requirements | |||
\**REQUIRES Python 3.7 | |||
- | |||
* - netCDF4 | |||
- >=1.5.4 | |||
- >=1.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 1.6.0 in METplotpy nco_requirements.txt and in plotpy requirements.txt, but 1.5.7 in calcpy requirements.txt and 1.6.2 in calcpy nco_requirements.txt. Should we update the calcpy requirements.txt and calcpy nco_requirements.txt or should the value here be 1.6.2 and should we update everywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's prudent to update to the latest version, so let's keep 1.6.2 and update in METplotpy and METcalcpy. I will put a note in METplotpy dtcenter/METplotpy#310 (update to Python 3.10 to use netCDF 1.6.2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bikegeek I defer to you on this one. If that is what you think is best, that works for me. This is PR approved. :)
@@ -380,7 +380,7 @@ METplus Components Python Requirements | |||
the netCDF C library | |||
- For using MET Python embedding functionality in use cases | |||
* - numpy | |||
- >=1.19.2 | |||
- >=1.23.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 1.22.0 in nco_requirements.txt, 1.22.3 in calcpy requirements.txt. I'm not sure which one we need and if we should update elsewhere.
@@ -394,7 +394,7 @@ METplus Components Python Requirements | |||
Fourier transforms, and more. | |||
- For using MET Python embedding functionality in use cases | |||
* - pandas | |||
- >=1.0.5, <=1.2.3 (METdataio) | |||
- >=1.5.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.5.1 in calcpy, plotpy and dataio nco_requirements.txt and 1.5.1 in plotpy and dataio requirements.txt, but 1.2.3 in calcpy requirements.txt. Should we update in calcpy requirements.txt?
@@ -409,15 +409,15 @@ METplus Components Python Requirements | |||
language | |||
- For using MET Python embedding functionality in use cases | |||
* - pint | |||
- >=0.18 | |||
- >=0.19.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 0.19.2 in calcpy and plotpy nco_requirements.txt and in plotpy requirements.txt, but 0.18 in calcpy requirements.txt. Should we update calcpy requirements.txt?
@@ -528,7 +528,7 @@ METplus Components Python Requirements | |||
libraries like Plotly | |||
- | |||
* - pyyaml | |||
- >=5.3.1 | |||
- >=6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 6.0 in calcpy, plotpy, and dataio nco_requirements.txt and 6.0 in plotpy and dataio requirements.txt, but 5.4.1 in calcpy requirements.txt. Should we update calcpy requirements.txt?
@@ -538,7 +538,7 @@ METplus Components Python Requirements | |||
programming language | |||
- | |||
* - scikit-image | |||
- >=0.16.2 | |||
- >=0.19.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 0.18.1 in calcpy requirements.txt. Should we updated calcpy requirements.txt?
@@ -572,7 +572,7 @@ METplus Components Python Requirements | |||
(scikit-learn) | |||
<../generated/model_applications/marine_and_cryosphere/GridStat_fcstRTOFS_obsSMAP_climWOA_sss.html>`_ | |||
* - scipy | |||
- >=1.5.1 | |||
- >=1.8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed as 1.8.1 in calcpy and plotpy nco_requirements.txt and 1.8.1 in plotpy requirements, but 1.8.0 in calcpy requirements.txt. Should we update calcpy requirements
The METcalcpy requirements did not get updated for the bugfix release (as
decided during one of the weekly meetings), so that will need to be
updated. But we will also need to update again for the Python 3.10, which
is why I think it was decided not to update it for the bugfix release???
…---------------
Minna Win
Pronouns: she/her
National Center for Atmospheric Research
Research Applications Lab
Developmental Testbed Center
Phone: 303-497-8423
Fax: 303-497-8401
---------------
On Fri, Feb 10, 2023 at 11:41 AM jprestop ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In docs/Users_Guide/overview.rst
<#2044 (comment)>:
> @@ -409,15 +409,15 @@ METplus Components Python Requirements
language
- For using MET Python embedding functionality in use cases
* - pint
- - >=0.18
+ - >=0.19.2
This is listed as 0.19.2 in calcpy and plotpy nco_requirements.txt and in
plotpy requirements.txt, but 0.18 in calcpy requirements.txt. Should we
update calcpy requirements.txt?
—
Reply to this email directly, view it on GitHub
<#2044 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4UJHQM6CE2CGFK6QL4MODWW2DUFANCNFSM6AAAAAAUXCB4XM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
cmocean, scikit-learn, and eofs are in the S2S use cases so will need to be
installed if running those use cases.
Cartopy is used by the physics tendency use case (under the short-range
examples). Are these requirements just for NCO, or for all users of
METplus?
…---------------
Minna Win
Pronouns: she/her
National Center for Atmospheric Research
Research Applications Lab
Developmental Testbed Center
Phone: 303-497-8423
Fax: 303-497-8401
---------------
On Fri, Feb 10, 2023 at 11:36 AM jprestop ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi @bikegeek <https://github.com/bikegeek>. Thank you for all of your
work on this task. For some of the versions changed below, I see some
discrepancies in the various requirements files. I'll put comments directly
on those items below. There are some other packages that weren't changed
that I have some questions about, which I'll list below in the comments
here:
*cartopy>=0.18.0* - I don't see it in the requirements.txt file for
METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or
METplotpy from the "METplus component" column. It looks like this is still
used in METplus wrappers for use cases.
*cmocean* - I don't see it in the requirements.txt file for METcalcpy or
METplotpy. If it is not being used, please remove this row entirely.
*eofs* - I don't see it in the requirements.txt file for METcalcpy or
METplotpy. If it is not needed, please remove METcalcpy or METplotpy from
the "METplus component" column. It looks like this is still used in METplus
wrappers for use cases.
*nc-time-axis 1.4* - I don't see it in the requirements.txt file for
METcalcpy or METplotpy. If it is not being used, please remove this row
entirely.
*scikit-learn 0.23.2* - I don't see it in the requirements.txt file for
METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or
METplotpy from the "METplus component" column. It looks like this is still
used in METplus wrappers for use cases.
*yaml* - Is this the same as pyyaml? I see that pyyaml lists
https://github.com/yaml/pyyaml as the source by yaml lists
https://pypi.org/project/PyYAML/. If they are the same, please pick
whichever line is most accurate and delete the other.
—
Reply to this email directly, view it on GitHub
<#2044 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4UJHXLJ6OBTDCIMO5DJDLWW2DETANCNFSM6AAAAAAUXCB4XM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Ah! I see. I totally missed that or forgot about it. Thank you for letting me know. |
Great question. No, they are not just for NCO, however, I saw METcalcpy and METplotpy listed in the "METplus Component" section so I was thinking that METcalcpy and METplotpy needed to be removed if that requirement was not for those components but was for METplus wrappers, but perhaps they're all intertwined? I really have no idea and defer to you on this. It just caught my attention so I thought I should mention it. Do you think this is all ok then? The only one I really wasn't sure about was netCDF4>=1.6.0. I thought maybe with the updates for NCO it needed to be 1.6.2 since it was 1.6.2 in calcpy nco_requirements.txt. |
METplotpy and METcalcpy are needed for a handful of the METplus use cases,
so I think it is OK to leave them documented. Good catch on the netcdf4, I
thought I was looking at the NCO requirements when I was updating the
versions. I'll update the version to 1.6.2.
---------------
Minna Win
Pronouns: she/her
National Center for Atmospheric Research
Research Applications Lab
Developmental Testbed Center
Phone: 303-497-8423
Fax: 303-497-8401
---------------
…On Fri, Feb 10, 2023 at 11:52 AM jprestop ***@***.***> wrote:
cmocean, scikit-learn, and eofs are in the S2S use cases so will need to be
installed if running those use cases. Cartopy is used by the physics
tendency use case (under the short-range
examples). Are these requirements just for NCO, or for all users of
METplus?
Great question. No, they are not just for NCO, however, I saw METcalcpy
and METplotpy listed in the "METplus Component" section so I was thinking
that METcalcpy and METplotpy needed to be removed if that requirement was
not for those components but was for METplus wrappers, but perhaps they're
all intertwined? I really have no idea and defer to you on this. It just
caught my attention so I thought I should mention it.
Do you think this is all ok then? The only one I really wasn't sure about
was netCDF4>=1.6.0. I thought maybe with the updates for NCO it needed to
be 1.6.2 since it was 1.6.2 in calcpy nco_requirements.txt.
—
Reply to this email directly, view it on GitHub
<#2044 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4UJHS2JUOGZOTWKWQBBJTWW2E6HANCNFSM6AAAAAAUXCB4XM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you! Once netCDF4 is updated to 1.6.2 I approve this request.
Hi @bikegeek. I just wanted to ensure that you knew that this is approve and ready for squashing and merging. According to the Merging pull requests section of our Contributor's Guide "As permissions allow, the requestor is responsible for merging the pull request once it has been approved." I do not know if you're ready to delete the branch, etc., so feel free to squash and merge when you're ready. |
…ools
Pull Request Testing
verify that in the ReadTheDocs User's Guide, the following versions are specified for Python 3.8.x:
lxml>=4.9.1
matplotlib >=3.5.2
metpy>=1.3.1
netcdf4?=1.6.0
numpy>=1.23.5
pandas>=1.5.1
pint>=0.19.2
plotly>=5.9.0
pymysql>=1.0.2
pytest>=7.2.0
pyyaml>=6.0
scikit-image>=0.19.3
scipy>=1.8.1
Do the same
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [NA]
Do these changes include sufficient testing updates? [NA]
Will this PR result in changes to the test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Please complete this pull request review by Bugfix release.
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s)
Select: Organization level software support Project or Repository level development cycle Project
Select: Milestone as the version that will include these changes